feat(server): add onPeerDisconnect, symmetric with onPeerConnect - #166
Conversation
startHttpAndWs's onDisconnected handler consumed the transport's own disconnect entirely (emitting the session-disconnected event), leaving callers with no way to observe a peer going away — the only socket-close hook is host-functions.ts's _emitSessionDisconnected, and its own docblock says it is @internal and must not widen the public surface. Add onPeerDisconnect(peer, meta), forwarded verbatim to the underlying startHttpAndWs. Unlike onPeerConnect it receives the raw session meta, not a wrapped session — by the time a peer disconnects there is no live RPC client left to attach. createDevServer never forwarded either hook down to startHttpAndWs, so this also adds a passthrough onPeerConnect option there for parity — today a createDevServer caller has no way to reach either callback at all.
✅ Deploy Preview for devfra ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
🟡 Changes recommended
onPeerDisconnect currently receives a meta object after internal disconnect cleanup mutates it, which can undermine the “raw session meta” semantics for consumers.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
This PR adds a public disconnect-time peer hook to the devframe node WebSocket server (startHttpAndWs) and plumbs both connect/disconnect peer hooks through createDevServer, enabling hosts to react to peer lifecycle events without reaching into @internal APIs.
Changes:
- Added
onPeerDisconnecttoStartHttpAndWsOptionsand invoked it from the WS transportonDisconnectedhandler. - Added
onPeerConnect/onPeerDisconnectpassthrough options tocreateDevServer. - Added vitest coverage for hook forwarding at both the
startHttpAndWsandcreateDevServerlayers.
File summaries
| File | Description |
|---|---|
| packages/devframe/src/node/server.ts | Adds onPeerDisconnect option and wires it into the WS disconnect hook. |
| packages/devframe/src/node/tests/server.test.ts | Verifies startHttpAndWs fires connect + disconnect hooks for the same session. |
| packages/devframe/src/adapters/dev.ts | Exposes and forwards peer hook options through createDevServer. |
| packages/devframe/src/adapters/tests/dev.test.ts | Verifies createDevServer forwards both hooks to startHttpAndWs. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
_emitSessionDisconnected -> streaming's _onSessionDisconnected clears meta.subscribedStreams in place. onPeerDisconnect ran after that, so consumers never saw the raw session meta the doc comment promises. Also updates the adapters/dev API snapshot for the new onPeerConnect/onPeerDisconnect options, which CI caught as stale.
There was a problem hiding this comment.
🟡 Changes recommended
A user-provided onPeerDisconnect callback can currently prevent internal disconnect cleanup from running if it throws, risking leaked per-session state.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (2)
packages/devframe/src/node/server.ts:237
onPeerDisconnectruns beforerpcHost._emitSessionDisconnected(meta). If a consumer callback throws, the internal disconnect cleanup (notably streaming cleanup) will be skipped becauseattachWsRpcTransportdoes not guardonDisconnected. Wrap the internal cleanup in afinallyso it always runs.
onDisconnected: (peer, meta) => {
options.onPeerDisconnect?.(peer, meta)
rpcHost._emitSessionDisconnected(meta)
},
packages/devframe/src/adapters/dev.ts:101
- The docstring says
onPeerDisconnectruns "right after its session's disconnect bookkeeping runs", butstartHttpAndWsinvokes the hook before devframe's own session cleanup (_emitSessionDisconnected). Update the wording to match the actual ordering/semantics.
/**
* Called once per closed WS connection, right after its session's
* disconnect bookkeeping runs. Forwarded verbatim to the underlying
* `startHttpAndWs`.
*/
- Files reviewed: 4/5 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
# Conflicts: # packages/devframe/src/node/__tests__/server.test.ts
What
startHttpAndWs'sonDisconnectedhandler consumed the transport's own disconnect entirely — it only fedrpcHost._emitSessionDisconnected(meta)— leaving callers with no way to observe a peer going away. The only socket-close hook that exists ishost-functions.ts's_emitSessionDisconnected, and its own docblock says it's@internaland "must not" widen the public surface. MeanwhileonPeerConnect(the connect-time counterpart) is already public.This adds
onPeerDisconnect?: (peer, meta) => void, forwarded verbatim to the underlyingstartHttpAndWs. UnlikeonPeerConnectit receives the raw session meta, not a wrapped session — by the time a peer disconnects there's no live RPC client left to attach.createDevServernever forwarded either hook down tostartHttpAndWsat all, so this also adds a passthroughonPeerConnectoption there for parity — today acreateDevServercaller has no way to reach either callback.Why this matters
Any host that tracks per-peer state outside the RPC layer (a registry keyed by connection, a presence list, cleanup for something the peer owned) currently has no first-party way to know a peer left — it either reaches for the internal, explicitly-not-for-this
_emitSessionDisconnected, or leaks that state forever.Tests
Two new cases:
packages/devframe/src/node/__tests__/server.test.ts:startHttpAndWsforwards both hooks for the same peer —onPeerConnectfires on open,onPeerDisconnectfires on close with matching session meta (meta.id), and neither fires early.packages/devframe/src/adapters/__tests__/dev.test.ts:createDevServerforwards both options down tostartHttpAndWsend to end.Both verified to fail (0 calls recorded) against the pre-fix code before adding the guard/forwarding.
pnpm --filter devframe exec vitest run— 49 files, 448 tests, all green.tsc --noEmitclean.eslintclean.